SOLR-3284: let ConcurrentUpdateSolrClient report which docs failed to send by serhiy-bzhezytskyy · Pull Request #4632 · apache/solr

serhiy-bzhezytskyy · 2026-07-10T14:55:46Z

https://issues.apache.org/jira/browse/SOLR-3284

Description

ConcurrentUpdateSolrClient sends updates asynchronously on background threads. When a batch fails, the only signal a caller gets is handleError(Throwable) — the exception, but not which documents didn't make it to the server. So a caller can't route the failed documents anywhere (retry queue, dead-letter topic); it only knows that something in the batch failed.

This is the pain SOLR-3284 has tracked since 2012, and it's still present on main (ConcurrentUpdateBaseSolrClient). We hit it in production indexing into Solr via the Builder, which is what prompted this.

The 2016 discussion on the JIRA (David Smiley, Mark Miller) converged on a Builder-configurable error handler, ideally a lambda. This implements that.

Changes

New UpdateErrorHandler functional interface: onError(Throwable ex, UpdateRequest request, String collection). It exposes the public UpdateRequest (and thus its documents), not the protected Update record, so it's usable by external callers. The caller reads whatever field is their uniqueKey — the client doesn't assume one (no hardcoded id).
New Builder.withErrorHandler(...). The default handleError(Throwable, Update) invokes the handler if one is registered, otherwise falls back to handleError(Throwable). With no handler the behavior is unchanged (logs), so this is backward compatible; existing handleError(Throwable) overrides keep working.
Restructured the runner loop so the failing Update is in scope and every failure path — HTTP error status and a thrown exception — reports it to the handler.

Usage:

new ConcurrentUpdateJdkSolrClient.Builder(url, httpClient)
    .withErrorHandler((ex, request, collection) -> {
        for (SolrInputDocument doc : request.getDocuments()) {
            // route the doc that didn't reach the server to retry / DLQ
        }
    })
    .build();

Two decisions I'd flag for review

Default behavior. I kept it optional + log-by-default for backward compatibility. The 2016 discussion leaned toward surfacing errors by default (throw when no handler is set, like SafeConcurrentUpdateSolrClient). I went with compatibility, but I'm happy to switch to throw-by-default if that's preferred — it's a small change.
Runner-loop behavior. To pass the failing request to the handler I catch failures per-update inside the runner loop. A side effect is that the runner now continues to the next batch instead of exiting the loop on error. This seems better (one bad batch no longer stalls the runner), but it is a behavior change and I want it visible rather than buried.

Side finding (not fixed here)

While testing I hit a pre-existing NPE: RemoteSolrException(host, code, remoteError) throws when remoteError is null, because the constructor does Map.of("remoteError", remoteError) and Map.of rejects null values. It's triggered when the server returns an error body that doesn't parse. Unrelated to this change and probably deserves its own issue — flagging it rather than expanding scope here.

Testing

New ConcurrentUpdateJdkSolrClientTest#testFailedDocsAreRecoverableViaErrorHandler: registers a handler via the Builder, sends 5 docs to a server returning 500, asserts all 5 doc ids are recovered.
./gradlew :solr:solrj:check :solr:solrj-jetty:check pass (solrj-jetty shares the base class).

dsmiley

Very nice improvement!

dsmiley · 2026-07-11T16:11:36Z

+  ConcurrentUpdateSolrClient can now report which documents failed to reach the server.
+  Register a handler via the Builder's withErrorHandler(...) to recover the documents of a
+  failed batch, for example to route them to a retry queue or dead-letter topic.
+type: added


That's the best changelog entry I've seen in a long time; thank you!

I think our project's embrace of the 3rd party "changelog" with it's choice of wording of "title" has led to a deterioration of changelog informative quality compared to before. You are bucking that trend. Thanks again. Our project seriously needs to find a way to use a different label here like "summary". CC @janhoy

Thanks — that means a lot. Agree the "title" label nudges people toward terse one-liners; "summary" would invite more.

dsmiley · 2026-07-11T16:22:38Z

    this.basePath = builder.baseSolrUrl;
    this.defaultCollection = builder.defaultCollection;
    this.pollQueueTimeMillis = builder.pollQueueTimeMillis;
+    this.errorHandler = builder.errorHandler;


IMO we probably want to make this new errorHandler the preferred mechanism... maybe even eventually mandatory in 11.

If we agree on this direction for 11 (I'm +1), then here we can use DeprecationLog to warn that a user forgot to pass an erroHandler, that it'll eventually be mandatory.

+1 on making the handler the preferred mechanism, and mandatory in 11. One wrinkle: DeprecationLog lives in solr-core (org.apache.solr.logging), so solrj can't reach it — wrong direction in the module graph. I can do the equivalent with a log.warn here when a client is built without a handler (and hasn't overridden handleError), worded as "will be required in a future release". Want me to go that way, or is there a solrj-side deprecation helper you'd prefer?

Confirmed there's no solrj-side deprecation helper — solrj only has @deprecated + ad-hoc log.warn, and DeprecationLog is core-only as noted. The one thing DeprecationLog adds over a bare log.warn is log-once dedup + the org.apache.solr.DEPRECATED.* logger prefix. To avoid warning on every client construction I'd add a small log-once guard here in the client. If the project later wants solrj to share core's deprecation convention, a tiny solrj-side helper would be a clean follow-up — but I'd keep this PR scoped to the guard. Sound good?

sounds good; thanks

serhiy-bzhezytskyy · 2026-07-12T05:26:42Z

Documented the handler's threading contract, with a test pinning it: the handler is invoked on runner threads, possibly concurrently — implementations must be thread-safe and should return quickly (a slow handler slows update throughput). An exception thrown by the handler is logged and does not stop the client — the runner's existing safety net already contains it; the test locks that promise in. No ordering guarantees across failures. No behavior change in this commit.

serhiy-bzhezytskyy · 2026-07-13T05:49:47Z

Moving the tests into ConcurrentUpdateSolrClientTestBase (so they run for both clients) surfaced a real bug on the Jetty client: it coalesces several queued updates into a single <stream> request, so a failed batch was only reporting the first update's documents — the rest were lost silently. The JDK client sends one request per update, so it was already fine.

The fix: doSendUpdateStream now returns the response together with the updates it actually sent (SentStream), and the error handler is invoked for every affected update, not just the first one in the coalesced batch. On the Jetty side the OutStream — which already is the batch boundary — now owns the updates coalesced into it, rather than tracking them in a parallel list. A handler that throws on one update is logged and doesn't stop the others or the runner.

dsmiley · 2026-07-13T13:30:59Z

+    // the updates coalesced into this stream, so a failure can report every affected one
+    private final List<ConcurrentUpdateBaseSolrClient.Update> updates = new ArrayList<>();


hmm; I'm concerned that this error tracking feature is going to hurt a benefit of CUSC -- memory efficiency (streaming a huge volume of updates).

What if we only track a designated ID field? The error handler could have a customizable overrideable method, defaulting to return the "id" field.

serhiy-bzhezytskyy · 2026-07-13T14:17:07Z

Switched it to report the failed document ids instead of the requests — onError(Throwable, List<String> ids, String collection) — so only ids are retained, not the documents, preserving the streaming memory efficiency.

The id comes from an overridable extractId(SolrInputDocument) defaulting to the id field, so a caller with a different uniqueKey can override it. When no handler is registered the documents are never read, so there's no overhead on the default path.

On the Jetty side the ids are collected as each doc is streamed, so a coalesced batch still reports every affected id on failure.

dsmiley

WDYT if the error handler also had a general error callback for other errors, apart from IDs? Default could simply log. The point is to have this callback handle errors comprehensively and furthermore no need to subclass the client.

dsmiley · 2026-07-13T17:33:50Z

+     * to the {@code id} field; override for a different uniqueKey.
+     */
+    default String extractId(SolrInputDocument doc) {
+      Object id = doc.getFieldValue(CommonParams.ID);


Just use "id" please. We've abused that CommonParams so much; it's supposed be only params.

dsmiley · 2026-07-13T17:36:34Z

+   * queued updates into one stream (e.g. the Jetty client), so it reports the ids of all of them
+   * here -- letting the error handler learn every failed document, not just the first. Only ids are
+   * retained (see {@link UpdateErrorHandler#extractId}), preserving streaming memory efficiency;


this wording here is indicative of a bug fix in the process of development but isn't useful in the delivered documentation. Of course "not just the first". AI loves to do this.
Likewise saying "only IDs are retains and why" is IMO not something to put here.

Trimmed to what the method does.

dsmiley · 2026-07-13T17:39:24Z

+  protected record SentStream(StreamingResponse response, List<String> docIds, String collection) {}
+
+  /** Factory for {@link SentStream}, callable by {@link #doSendUpdateStream} implementations. */
+  protected static SentStream sentStream(


please rename newSentStream

or just remove this extracted method... I don't see the point of it. If it wasn't static, I might surmise why you did this.

Removed it — made the record public instead: a protected record constructor isn't reachable from the Jetty package.

dsmiley · 2026-07-13T17:45:07Z

+  /** Extracts a document id via the registered handler (defaults to the {@code id} field). */
+  protected String extractId(SolrInputDocument doc) {
+    return errorHandler == null ? null : errorHandler.extractId(doc);
+  }


this is a fragile relationship IMO. As-written, this method is only useful if it's being used for error tracking purpose. But it's name is not indicative of that, so it might eventually be used for logging or whatever other purposes. One solution is to inline the logic. Another solution is to make this method the place where the ID is actually extracted... and so remove that responsibility from the error handler. A rare user who doens't use "id" must subclass this method.

Delegate removed; helper renamed idsForErrorReporting with the empty-when-no-handler contract documented.

dsmiley · 2026-07-13T17:46:38Z

+  protected List<String> docIds(UpdateRequest request) {
+    if (errorHandler == null || request.getDocuments() == null) {
+      return List.of();
+    }


again; I see a fragile relationship. A method with a straight-forward name yet is unusable outside of error-tracking purposes.

Delegate removed; helper renamed idsForErrorReporting with the empty-when-no-handler contract documented.

dsmiley · 2026-07-13T17:49:15Z

+          } catch (Throwable e) {
+            if (e instanceof OutOfMemoryError) {
+              throw (OutOfMemoryError) e;
+            }
+            if (e instanceof InterruptedException) {
+              throw (InterruptedException) e;
+            }


Can't we catch these and throw without instanceof?

note: java lang 17 here

dsmiley · 2026-07-13T18:00:25Z

+
+    String serverUrl = solrTestRule.getBaseUrl() + "/cuss/foo";
+    List<String> failedIds = new CopyOnWriteArrayList<>();
+


lets improve this slightly to add a doc successfully, call blockUntilFinished, then proceed to mess things up and only have the failed docs get reported.

Note this would mean switching from a bogus URL to real Solr but put fields on invalid docs that don't fit the schema. TestServlet isn't needed in this test, I think.

Rewritten per your sketch: good doc first, then schema-rejected docs; only the failures are reported.

…ed to send Add a Builder-configurable error handler so callers can recover the documents of an update batch that failed to reach the server. handleError(Throwable) alone only gave the exception, not which documents were lost, so callers could not route the failed docs to a retry queue or dead-letter topic. - New UpdateErrorHandler: onError(Throwable, List<String> failedIds, String collection) reports the ids of the failed documents. Ids are obtained via an overridable extractId(SolrInputDocument) that defaults to the "id" field, so callers whose uniqueKey differs can override it. Registered via Builder.withErrorHandler(...). With no handler the client keeps its existing log-only behavior and never reads the documents, so this is backward compatible and preserves the client's streaming memory efficiency (only ids are retained, not the documents). - doSendUpdateStream returns SentStream(response, ids, collection). The Jetty client coalesces several queued updates into one stream, so it collects the ids of all of them -- the handler receives every affected id on failure, not just the first. The JDK client sends one request per update. - An exception thrown by the handler is logged and does not stop the runner. Tests live in ConcurrentUpdateSolrClientTestBase so they run for both the JDK and Jetty clients.

dsmiley · 2026-07-13T19:45:40Z

Please don't force-push to PRs, especially once someone has looked at it. It resets the GH review state for the reviewer, making it impossible to be sure what changed since the last reviewed changes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

serhiy-bzhezytskyy · 2026-07-13T20:01:00Z

Split the handler into two callbacks, per your suggestion. onError(ex, ids, collection) reports the failed document ids and stays the lambda target. A default onError(ex) handles errors not tied to specific documents (e.g. a failure before any doc was sent) — it logs by default, so a registered handler can't silently swallow those. That was the hole in the single-method version: with an empty id list a lambda user got nothing. No subclassing needed for either.

Two smaller things from earlier:

SentStream is public now instead of the factory — a protected record constructor isn't reachable from the Jetty client's package, and public is simpler than the workaround.
id extraction stayed on the handler as idOf(doc) so callers with a different uniqueKey can override it without subclassing the client. Renamed the client-side helper to idsForErrorReporting so it reads as what it does — and documents that it's empty when no handler is registered (documents are never read on the default path).

Both transport suites are green.

serhiy-bzhezytskyy · 2026-07-13T20:07:46Z

Fair — my mistake, and it won't happen again: incremental commits only from here on. The latest javadoc trim already went as a plain commit.

dsmiley

Looks good.
Glad to see that moving the tests to the base led to a bug discovery and ultimately more robustness.

serhiy-bzhezytskyy · 2026-07-14T04:15:17Z

Thanks for the review. The move to the base class was the turning point — it surfaced the Jetty coalescing bug I'd never have caught with a JDK-only test. I went further into the transports than I expected, even tried a listener-style variant that didn't earn its place. The ids-only handler from your memory-efficiency point is lighter and happens to be exactly what my own prod indexing needs. Happy to pick up the preferred / eventually-mandatory follow-up for 11 when it's time.

github-actions Bot added client:solrj tests labels Jul 10, 2026

dsmiley reviewed Jul 11, 2026

View reviewed changes

dsmiley requested a review from markrmiller July 11, 2026 16:27

dsmiley reviewed Jul 12, 2026

View reviewed changes

Comment thread solr/solrj/src/java/org/apache/solr/client/solrj/impl/ConcurrentUpdateBaseSolrClient.java Outdated

Comment thread solr/solrj/src/test/org/apache/solr/client/solrj/impl/ConcurrentUpdateJdkSolrClientTest.java

serhiy-bzhezytskyy force-pushed the solr-3284-cusc-failed-docs branch from 5d9bf45 to 1f652f6 Compare July 13, 2026 05:39

dsmiley reviewed Jul 13, 2026

View reviewed changes

dsmiley requested a review from jdyer1 July 13, 2026 13:33

serhiy-bzhezytskyy force-pushed the solr-3284-cusc-failed-docs branch from 1f652f6 to 7392d54 Compare July 13, 2026 14:14

serhiy-bzhezytskyy force-pushed the solr-3284-cusc-failed-docs branch 3 times, most recently from c4e87b1 to 7cf129b Compare July 13, 2026 15:01

dsmiley reviewed Jul 13, 2026

View reviewed changes

serhiy-bzhezytskyy force-pushed the solr-3284-cusc-failed-docs branch from 7cf129b to 8903e4c Compare July 13, 2026 19:19

serhiy-bzhezytskyy force-pushed the solr-3284-cusc-failed-docs branch from 8903e4c to bcd15dc Compare July 13, 2026 19:45

SOLR-3284: trim onError(Throwable) javadoc to what it does

f70d6ae

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

dsmiley approved these changes Jul 14, 2026

View reviewed changes

dsmiley merged commit 14e8503 into apache:main Jul 14, 2026
6 checks passed

dsmiley added this to the 10.x milestone Jul 14, 2026

		// the updates coalesced into this stream, so a failure can report every affected one
		private final List<ConcurrentUpdateBaseSolrClient.Update> updates = new ArrayList<>();


		String serverUrl = solrTestRule.getBaseUrl() + "/cuss/foo";
		List<String> failedIds = new CopyOnWriteArrayList<>();

Uh oh!

Conversation

serhiy-bzhezytskyy commented Jul 10, 2026

Description

Changes

Two decisions I'd flag for review

Side finding (not fixed here)

Testing

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

serhiy-bzhezytskyy commented Jul 12, 2026

Uh oh!

Uh oh!

Uh oh!

serhiy-bzhezytskyy commented Jul 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-bzhezytskyy commented Jul 13, 2026

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsmiley commented Jul 13, 2026

Uh oh!

serhiy-bzhezytskyy commented Jul 13, 2026

Uh oh!

serhiy-bzhezytskyy commented Jul 13, 2026

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-bzhezytskyy commented Jul 14, 2026